feat!(detector): route Cisco to vuls2#2581
Conversation
There was a problem hiding this comment.
Pull request overview
Routes Cisco-derived CPE detection and Cisco CveContent enrichment through the vuls2 DB path (matching prior NVD/JVN migrations), and removes Cisco filling/synthesis from the go-cve-dictionary path so Cisco detection/enrichment is sourced consistently from vuls2.
Changes:
- Drops Cisco-derived
CveContentbuilt from the generic CPE vulnerability stub and enriches Cisco from advisory roots via a newenrichCisco. - Expands vuls2 enrichment datasource filtering to include Cisco advisory sources (CSAF/CVRF/JSON) and adds a Cisco enrichment test + fixtures.
- Removes Cisco
CveContentfill and go-cve-dictionary-side Cisco advisory synthesis, and ensures Cisco-only dictionary detections are skipped so vuls2 re-detects them.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| detector/vuls2/vuls2.go | Drops sparse Cisco CveContent created during vulninfo creation; adds Cisco sources to the vuls2 enrich datasource filter. |
| detector/vuls2/vendor.go | Wires Cisco advisory sources into advisory enrichment via enrichCisco. |
| detector/vuls2/vuls2_test.go | Adds a Cisco enrichment test case asserting advisory-sourced content and no CVSS. |
| detector/vuls2/testdata/fixtures/enrich/cisco-json/datasource.json | Adds Cisco datasource fixture metadata for enrichment tests. |
| detector/vuls2/testdata/fixtures/enrich/cisco-json/data/2099/cisco-sa-test-abc.json | Adds Cisco advisory+vulnerability fixture to drive enrichCisco test coverage. |
| detector/detector.go | Removes Cisco CveContent fill and dictionary-side Cisco advisory synthesis; adjusts dictionary CPE detection gating/stripping for Cisco migration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on PR #2581: - enrichAdvisories selected the Cisco source via a.Contents map iteration, which Go randomizes; when an advisory carries multiple Cisco source representations (CSAF/CVRF/JSON) the winner was non-deterministic. Iterate a fixed priority order (CSAF > CVRF > JSON, mirroring compareSourceID) instead. - Correct the FillCvesWithGoCVEDictionary doc comment: NVD/US-CERT alerts come from the vuls2 enrich path; only JP-CERT (from JVN) still comes from here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Copilot review on PR #2581: the priority loop kept calling enrichCisco for every Cisco source present even though it no-ops once the cisco CveContent exists. Break after the first source that populates it — fewer redundant calls and clearer "first by priority" intent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Copilot review on PR #2581: - enrichAdvisories doc comment now notes it enriches Cisco CveContent too, not just KEV. - enrichCisco's "already present" note no longer cites the CPE detection path as the example (that path's sparse Cisco content is now deleted); make it generic (a higher-priority Cisco source on a prior call). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Copilot review on PR #2581: - enrich() now stamps vi.CveID from the map key before running enrichers, so enrichers that derive CveContent.CveID from vi.CveID (enrichCisco) never emit an empty CveID when a caller relied on the map key. - enrichCisco tags *.cisco.com reference URLs with Source "CISCO" instead of toReference's default "MISC", matching go-cve-dictionary's ConvertCiscoToModel and the Cisco advisoryReference so reference-source grouping stays stable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…empty Address Copilot review on PR #2581: - postConvert: look up advContent[src] with the comma-ok form; only build the Cisco CveContent from it when present, and reuse the already-computed cc.Optional instead of recomputing cveContentOptional. If a Cisco detection has no matching advisory content (anomalous for cisco-json, whose advisory shares the root), emit no cisco content so the enrich path backfills it by CVE rather than shipping an empty entry. - enrichCisco: skip only when a non-empty cisco content is already present (the detection-built, provenance-bearing one); an empty/placeholder entry is still backfilled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Copilot review on PR #2581: Test_enrich only exercised enrichCisco (the backfill), not the postConvert detection branch. Add a Test_postConvert case with a minimal CiscoJSON advisory + CVE-stub vulnerability + CPE detection that asserts the Cisco CveContent is built from the advisory (title/summary/ source link/CISCO references), carries Optional["vuls2-sources"] provenance, and that the empty vulnerability stub does not leak into the content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Migrate Cisco (cisco-json/csaf/cvrf) CPE vulnerability detection and content from go-cve-dictionary to the vuls2 boltdb. - postConvert builds Cisco CveContent from the advisory (vulnerability is a thin CVE stub), carrying the vuls2-sources provenance; Cisco has no CVSS (vendor SIR -> DistroAdvisory severity), references to *.cisco.com are tagged "CISCO". - enrichAdvisories backfills Cisco content for CVEs detected by other sources, selecting the source deterministically by priority (CSAF > CVRF > JSON); enrich DataSources include the Cisco sources. - mergeVulnInfo keys CveContents by (type, source link) so a CVE under multiple Cisco advisories keeps all of them. - detector.go drops Cisco from the go-cve-dictionary admission gate / content fill and strips it before getMaxConfidence, so Cisco is reported by vuls2 only. - tui: treat gocui ErrQuit as a clean exit (staticcheck SA4023). Squashed from the shino/cpe-detect-cisco review history (was PR #2581). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…chCisco enrichCisco guarded on a non-empty cisco entry and then deleted the cisco key before backfilling. Since CveContent.Empty() is Summary=="", a detection-built Cisco content for an advisory with an empty description was treated as empty, deleted, and rebuilt with nil Optional — silently dropping the vuls2-sources provenance that only the detection path can attach. Switch to a presence-based skip (matching enrichNVD / enrichRedHatCVE): if any cisco content is already present, leave it untouched. The detection path never writes empty placeholders (it emits no cisco key when it has nothing), so the backfill still runs for CVEs the Cisco CPE path did not detect. Add regression tests: - Test_mergeVulnInfo: a CVE with two Cisco advisories keeps both CveContents (the (type, source link) merge key), instead of collapsing by type. - Test_enrich case: enrichCisco skips a CVE that already carries detection-built cisco content, preserving its vuls2-sources provenance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s/fixtures vulsio/vuls-data-update#844 (cisco-json extractor) is merged. Replace the synthetic Test_postConvert Cisco case (cisco-sa-test / cisco:test_product) with a real extracted advisory from the merged golden: cisco-sa-3100_4200_tlsdos (CVE-2025-20127, cpe:2.3:a:cisco:firepower_threat_defense, vendor SIR High, CWE-404, cisco.com references, distinct published/modified dates). The product version (7.4.0.0) is paren-free, keeping CPE strings clean; the long advisory description is trimmed to one real sentence. Align the cisco-json fixture datasource name with the extractor's output ("Cisco Security Advisories: openVuln API"). The n39k enrich fixture content already matches the merged golden (only the canonical raws path differs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ontentsFromAdvs The vulnerability-stub CveContent literal lived inline in postConvert's vuln loop, deeply indented and long. Move it into cveContentsFromVulns in vendor.go so the source-origin switch reads symmetrically: each arm is a one-line call (cveContentsFromVulns for stub-sourced, cveContentsFromAdvs for advisory-sourced). The vuls2-sources provenance (optional) is now computed once before the switch and passed to both. References are still merged in the caller (the advisory-ref merge can fail) and handed in, so cveContentsFromVulns stays error-free and symmetric with cveContentsFromAdvs. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…romVulns Follow-up to the cveContentsFromVulns extraction: also move the CVSS computation (toCvss) and the reference merge (vulnerability refs + the source's advisory references, the part that can fail) into cveContentsFromVulns — both are used only on the vulnerability-sourced path. It now takes (src, cctype, v, fdas, optional) and returns (CveContents, error); cveContentsFromAdvs gains a matching (CveContents, error) signature (nil error today) so the two read symmetrically. The postConvert vuln loop now builds the VulnInfo first and assigns vinfo.CveContents from the switch, dropping the local cvss/rs/ccs vars and the inline reference-merge loop; a single error check follows the switch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…switch arm Move the error check into each switch arm (a local ccs, err := ... then assign vinfo.CveContents) instead of a single post-switch check that reassigned the outer err. Slightly more verbose (one temp per arm) but keeps each arm's error handling self-contained and the failure message specific. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RunTui logged gocui.ErrQuit (the normal-quit sentinel) as an error and called os.Exit(1), so a normal TUI quit looked like a failure and bypassed the deferred g.Close() cleanup. Gate on errors.Is(err, gocui.ErrQuit): return ExitSuccess on a normal quit and ExitFailure (logged) only for a real error, letting the deferred Close run in every case. Dropping the always-true `err != nil` also removes the staticcheck SA4023 suppression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lean exit Refine the previous tui change: still treat gocui.ErrQuit (normal quit) as a clean exit so a normal quit no longer logs an error or exits 1 (and the deferred g.Close runs), but for a real error keep the original behavior — explicit g.Close, log, os.Exit(1) — rather than returning ExitFailure, to avoid changing the error-path exit semantics. Using errors.Is means there is no always-true `err != nil` comparison, so no staticcheck SA4023 to suppress. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Design question: should Cisco synthesize a Stepping back from the Cisco's data is advisory-shaped: the Proposal: represent Cisco as a Stays intact:
Conscious trade-off (lost):
Net: Cisco becomes "detection + WDYT? Happy to be talked out of it if some consumer specifically relies on the Cisco |
…p synthetic CveContent Per review (MaineK00n on #2581): Cisco data is advisory-shaped — the vulnerability entry is a bare CVE-ID stub, all content lives in advisories[], and CVE↔advisory is M:N — which is exactly what DistroAdvisory models. Forcing it into a per-CVE CveContent is what required the special-casing this PR added, and the M:N-in-CveContent is what caused the enrichCisco multi-advisory bug. Represent Cisco as detection + DistroAdvisory only, and drop the synthetic Cisco CveContent entirely. Removed: - ciscoCveContent, cveContentsFromAdvs, enrichCisco - the advisory-content stash in walkVulnerabilityDatas - the enrichAdvisories Cisco case and CiscoJSON from enrich's DataSources - the (type, SourceLink) re-keying of mergeVulnInfo (back to keying by type, since no source emits multiple same-type contents anymore) The vulnerability loop now skips CveContent synthesis when cctype == Cisco; detection (CPE match / confidence), the advisory ID/dates, and SIR -> DistroAdvisory.Severity are unchanged, and go-cve-dictionary's Cisco is still stripped (vuls2 owns Cisco detection). Summaries()/Titles() already fall back to DistroAdvisories, so descriptions still render. Conscious trade-off (documented): Cisco-sourced CWE and the advisory reference list (bug/csaf/cvrf URLs) drop for Cisco-only CVEs, since those are aggregated only from CveContents; for CVEs also in NVD/MITRE they return via enrich. This is a deliberate departure from go-cve-dictionary parity. Tests: postConvert Cisco case now asserts DistroAdvisory + no CveContent; the enrichCisco and multi-advisory mergeVulnInfo tests (and the cisco-json enrich fixture) are removed as their behavior no longer exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Adopted — thanks, this is clearly the right shape. Pushed in b408f38. Cisco is now detection +
The vuln loop skips Trade-off noted in the commit message: Cisco-sourced CWE and the advisory reference list drop for Cisco-only CVEs (aggregated only from Net diff: +35 / -326. The inline |
…ot enriched The enrich doc comment still listed Cisco among the enriched sources, but Cisco was dropped from enrich (DataSources + enrichAdvisories) when it became DistroAdvisory-only. Update the comment to match: enrich covers KEV/RedHat/NVD/ EUVD/MITRE/exploits; Cisco is detection-only (DistroAdvisory). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s master cveContentsFromVulns was extracted from walkVulnerabilityDatas only to be symmetric with cveContentsFromAdvs. The latter is gone (Cisco is DistroAdvisory- only), so the extraction has no rationale left. Inline it back so the master CVE content literal is restored verbatim and the only walk-loop change is the `if cctype != models.Cisco` guard around it. Also revert the gratuitous am→distroAdvs rename, the mergeVulnInfo comment, and the ccm loop-var rename, and keep the enrich doc comment as master's wording plus the one-line Cisco note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Express the "Cisco emits no per-CVE CveContent" rule as a switch on cctype (empty case models.Cisco, default builds the content) instead of a negated if, so the advisory-shaped exception reads as its own labelled branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…k to advisory Per maintainer review, drop the DistroAdvisory-only special case: Cisco now flows through the same default detection path as every other CPE source and builds a (sparse) per-CVE CveContent from the vulnerability stub. This also removes the walkVulnerabilityDatas Cisco switch entirely, so that function and enrich return byte-identical to master. Cisco content lives in the advisory rather than a per-CVE page, so the CveContent source link points at the advisory: cveContentSourceLink now takes the root ID (which is the Cisco advisory ID) and returns the advisory URL for models.Cisco. The three enrich callers thread their root ID through unchanged behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
After Cisco started emitting a sparse per-CVE CveContent in the detection path, two comments still described it as DistroAdvisory-only: the postConvert test case header and the FillCvesWithGoCVEDictionary doc. Update both to match the actual behaviour (DistroAdvisory + sparse CveContent with an advisory source link). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
Move Cisco-derived data from go-cve-dictionary to the vuls2 path, mirroring the NVD (#2575) and JVN migrations. Cisco CPE detection now comes from vuls2; go-cve-dictionary no longer detects or fills Cisco.
Cisco flows through the same detection path as every other CPE source: it produces a
DistroAdvisory(AdvisoryID + vendor SIR severity) and a per-CVECveContent. Cisco data is advisory-shaped — the vulnerability entry is a bare CVE-ID stub and the real content lives in the advisory — so theCveContentis sparse (no CVSS; Cisco carries only a vendor SIR, surfaced as theDistroAdvisoryseverity) and its source link points at the advisory page rather than a per-CVE page.Why
Continue routing CPE-ecosystem sources through vuls2 (cpematch-expanded criteria, unified match-quality projection). Cisco is the next source after NVD/JVN.
How
Detection
detectCpeURIsCvesWithGoCVEDictionarydrops Cisco from the admission gate and nilsdetail.Ciscosbefore confidence selection (same treatment as NVD), so Cisco-only details are skipped and vuls2 re-detects them.walkVulnerabilityDatasis unchanged from master: Cisco is detected like any other CPE source, building theDistroAdvisoryfrom the advisory roots and the per-CVECveContentfrom the vulnerability stub.cveContentSourceLinkgains amodels.Ciscocase. Cisco content lives in the advisory, whose ID is the detection root ID, so the per-CVE source link is the advisory URL (https://sec.cloudapps.cisco.com/security/center/content/CiscoSecurityAdvisory/<advisoryID>). The helper now takes the root ID; the existing NVD/RedHat/MITRE callers thread their root ID through with unchanged behaviour.Enrich
enrich()'sDataSourcesfilter and fromenrichAdvisories.go-cve-dictionary cleanup
FillCvesWithGoCVEDictionaryno longer fills Cisco.DistroAdvisory(AdvisoryID + SIR) is now produced by the vuls2 detection path, so the go-cve-dictionary synthesis is removed. No NVD-coverage gating is needed for Cisco (it was always unconditional).Dependencies
db-nightly.mk). Until then the Cisco data must bevuls db add-ed locally.Testing
postConverttest: a Cisco CPE detection emits aDistroAdvisoryplus a sparseCveContentwhose source link is the advisory URL (no CVSS/title/summary, since the vulnerability stub carries only the CVE-ID).:nightlyDB: same CVE set with a matching SIRDistroAdvisoryper CVE.🤖 Generated with Claude Code